-
-
Notifications
You must be signed in to change notification settings - Fork 802
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CallBase
should not be allowed for delegate mocks
#708
CallBase
should not be allowed for delegate mocks
#708
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tehmantra - Thank you for submitting this PR, it looks very good already! 👍 See the below review comments for a few minor change requests. Also, could you please add an entry in the changelog, e.g. as follows:
#### Fixed
* `NullReferenceException` when using `SetupSet` on indexers with multiple parameters (@idigra, #694)
+* <short description of your fix, e.g. your PR's title> (@tehmantra, #708)
## 4.10.0 (2018-09-08)
Thank you!
src/Moq/MethodCall.cs
Outdated
@@ -123,6 +123,11 @@ public override void Execute(Invocation invocation) | |||
|
|||
public virtual void SetCallBaseResponse() | |||
{ | |||
if (typeof(Delegate).IsAssignableFrom(this.Mock.TargetType)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moq defines an extension method Type.IsDelegate()
, could you please use it here? → this.Mock.TargetType.IsDelegate()
src/Moq/Mock.Generic.cs
Outdated
set => this.callBase = value; | ||
set | ||
{ | ||
if (typeof(Delegate).IsAssignableFrom(this.MockedType)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(As above:) Moq defines an extension method Type.IsDelegate()
, could you please use it here?
src/Moq/Mock.Generic.cs
Outdated
{ | ||
if (typeof(Delegate).IsAssignableFrom(this.MockedType)) | ||
{ | ||
throw new NotSupportedException(string.Format(CultureInfo.CurrentCulture, Resources.CallBaseUsedOnDelegateException)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is important that an exception be thrown only if value == true
. This is because when you have a mock with DefaultValue.Mock
, and Moq auto-mocks something (e.g. the delegate mock's return value), it'll set that new mock's CallBase
to the same value as the "parent" mock (here).
@@ -87,6 +87,15 @@ internal class Resources { | |||
} | |||
} | |||
|
|||
/// <summary> | |||
/// Looks up a localized string similar to CallBase cannot be used to mock Delegate types.. | |||
/// </summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There appears to be something wrong with indentation here, could you please take a look & fix this?
src/Moq/Properties/Resources.resx
Outdated
<data name="CallBaseUsedOnDelegateException" xml:space="preserve"> | ||
<value>CallBase cannot be used to mock Delegate types.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest a slight rephrasing here:
-CallBase cannot be used to mock Delegate types.
+CallBase cannot be used with Delegate mocks.
(The reason for rephrasing being that CallBase
isn't the party that actively mocks something; the delegate mock exists already by the time CallBase
is being set.)
* Update CHANGELOG * Use .IsDelegate() extension * Only throw if CallBase == true * Fix whitespace (spaces to tabs) * Fix CallBase error name and description
All comments resolved. Cheers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes! One last thing, which I only discovered now (sorry for not noticing it earlier): .CallBase()
still works for non-void
delegate mocks:
var mock = new Mock<Func<bool>>();
mock.Setup(m => m()).CallBase(); // no exception gets thrown
which is likely because another this.Mock.TargetType.IsDelegate()
check is missing here:
Could you please add the missing check, along with an additional unit test verifying this scenario?
After this, we should be ready to merge. Thanks once again!
Done. Missed that this has 2 implementations! Cheers. |
Awesome! Thank you for contributing! |
@tehmantra - Moq 4.10.1, which includes your bugfix, has just been published on NuGet. |
Fixes: #706
First time looking at this codebase, I've tried to follow the style as best as I could.
I added tests that capture the issue, hopefully I have understood it correctly.
Cheers